Fix #798, Update build/test workflows to include sample/lab apps #799
Conversation
There was a problem hiding this comment.
See my comments in the code. I like the concept but we never documented that the words "sample" or "lab" will never appear in app names outside of the framework apps.
I'd like to see if this can more directly test if the app name is already in the respective lists, or maybe instead of patching the sample, could it generate the targets.cmake and/or cfe_es_startup.scr with the content specifically for that app?
| run: | | ||
| echo "APP_UPPER=$(echo ${{ inputs.app-name }} | sed 's/[a-z]/\U&/g')" >> $GITHUB_ENV | ||
| echo "APP_LOWER=$(echo ${{ inputs.app-name }} | sed 's/[A-Z]/\L&/g')" >> $GITHUB_ENV | ||
| echo "IS_SAMPLE_OR_LAB=$(echo ${{ inputs.app-name }} | grep -iqE 'sample|lab' && echo true || echo false)" >> $GITHUB_ENV |
There was a problem hiding this comment.
My only (slight) concern is with this logic. I can see why you did this -- that the "sample" and "lab" apps are already in the sample config, thus we need to not add them again or else we'd end up with a duplicate entry.
My concern is that this will break if someone changes the sample config such that this assumption is not true anymore. I also don't want to assume that there will never be another app with either "sample" or "lab" in its name.
Could we do something more direct -- that is, instead of checking the app name, check if the script already has the entry? That is, if running on "to_lab" -- just check if the cfe_es_startup.scr already has an entry for to_lab, and skip the update if it does?
There was a problem hiding this comment.
At the very least -- another idea is could this be a parameter that is passed into the workflow? Something like "is_framework_app" ... with a default value of false. Then when TO/CI/SCH Lab and Sample App invoke this workflow, they can pass it as true, but everything else remains as it was. This would accomplish the same goal without making this assumption.
Checklist
Describe the contribution
unit-test-coverage.ymlandbuild-run-app.ymlwith the sample_app, sample_lib and the lab apps.Testing performed
GitHub CI actions all passing successfully (incl. Build + Run, Unit/Functional Tests etc.).
I've tested the build-run-app workflow with sample_app, sch_lab and sample_lib, and the unit-test-coverage workflow with sample_app and sample_lib - all working as expected.
Expected behavior changes
This PR will allow immediate notification of a regression or issue with the build/tests in the sample/lab repositories when pushing a branch or creating a PR.
System(s) tested on
Debian 12 using the current main branch of cFS bundle.
Contributor Info
Avi Weiss @thnkslprpt